Skip to content

split the actual check implementations from the instances#5323

Merged
firewave merged 2 commits into
cppcheck-opensource:mainfrom
firewave:checks-xxy
May 28, 2026
Merged

split the actual check implementations from the instances#5323
firewave merged 2 commits into
cppcheck-opensource:mainfrom
firewave:checks-xxy

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Aug 13, 2023

No description provided.

@firewave

This comment was marked as outdated.

@firewave firewave force-pushed the checks-xxy branch 4 times, most recently from 429f3eb to 876e259 Compare November 6, 2023 01:34
@firewave firewave force-pushed the checks-xxy branch 5 times, most recently from ba3fb72 to 28cfb23 Compare November 20, 2023 00:38
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Apr 4, 2025

A side note - outside of the tests we would actually not require any of the checks to have headers at all since the application code is only accessing them through the instances.

@firewave firewave force-pushed the checks-xxy branch 11 times, most recently from 7423146 to 0f3e4ff Compare May 18, 2026 13:35
Comment thread lib/checkimpl.cpp Fixed
Comment thread lib/checkimpl.cpp Fixed
Comment thread lib/checkmemoryleak.cpp Fixed
Comment thread lib/checkimpl.cpp Fixed
Comment thread lib/checkuninitvar.cpp Fixed
@firewave firewave force-pushed the checks-xxy branch 5 times, most recently from eb05ce8 to 1aaf437 Compare May 19, 2026 06:32
@firewave firewave changed the title extracted code for actual check implementation from Check into CheckImpl split the actual check implementations from the instances May 19, 2026
@firewave
Copy link
Copy Markdown
Collaborator Author

The check objects were ambiguous in their purpose. The constructor with the name only constructed the instance which had all internal pointer set to NULL and should never be used to run the checks. The other constructor was used to generate the actual object for the analysis. This splits the checks into separate instance and implementation classes.

This will allow us to change the pointer to references (a follow-up I have already lined up) and also clean up some interfaces (more follow-ups).

There is most likely some additional cleanups possible with the access level of the implementations (non-analysis function should probably not be public - well, actually none of them should probably be public in the production code but I would not like to introduce friend declarations as that would mess with the unused function detection - something to re-visit later on - see also below).

A side note - outside of the tests we would actually not require any of the checks to have headers at all since the application code is only accessing them through the instances.

This is no longer the intended goal. The idea was to hide the implementation completely and only use the interface. But with umbrella checks like CheckOther that doesn't work because we do not want to run unit tests as a whole on those but only specific checks. Actually we should be doing this for all unit tests and create the *Impl and only call the checks we actually want to test (that is a further follow-up).

@firewave firewave marked this pull request as ready for review May 19, 2026 07:55
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented May 28, 2026

I know you hate AI but it's hard to review such big change manually. AI has this feedback:

The Impl suffix is universally associated with the PIMPL (pointer-to-implementation) pattern, where FooImpl is a hidden private type
owned by Foo. Here, CheckAssertImpl is completely unrelated to CheckAssert via inheritance — it is a sibling, not a detail. This will
mislead contributors who see CheckAssertImpl and look for a CheckAssert
wrapper owning it. Consider CheckAssertRunner, or keeping the original name for the implementation class and using CheckAssertReg for the
registration stub

Any opinions about this? Personally I have never used PIMPL. So I am not confused..

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented May 28, 2026

Another question:

  1. getErrorMessages() is still in Check*, not CheckImpl
    The error-message strings are defined in the same methods that call reportError(). But reportError() now lives in CheckImpl. For getErrorMessages(), a null ErrorLogger
    trick was historically used — with the split, does Check*::getErrorMessages() instantiate a Check*Impl internally? If not, how is reportError() reached? This needs to be clearly shown in the PR or documented; it's a non-obvious dependency.

@firewave
Copy link
Copy Markdown
Collaborator Author

Any opinions about this? Personally I have never used PIMPL. So I am not confused..

It is just called *Impl - we are not using PIMPL here.

with the split, does Check*::getErrorMessages() instantiate a Check*Impl internally?

Obviously. The code in those function has not changed at all - we just split the instance code from the implementation one so the Check* objects are no longer ambiguous in what they can be used for.

@firewave firewave merged commit f1379d6 into cppcheck-opensource:main May 28, 2026
70 checks passed
@firewave firewave deleted the checks-xxy branch May 28, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants